Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Algae.Maybe.new(nil) return a Nothing #27

Merged
merged 2 commits into from
Jul 4, 2018
Merged

Make Algae.Maybe.new(nil) return a Nothing #27

merged 2 commits into from
Jul 4, 2018

Conversation

icidasset
Copy link
Member

Was this what you had in mind?
Can also write some more tests if you want me to ☺️

@OvermindDL1
Copy link

OvermindDL1 commented Jun 13, 2018

Uh, a Maybe.new(...) returning anything but a valid non-Nothing value seems weird to me... nil can be an entirely valid value to return, like the result of a computation that succeeded.

Now Maybe.new() returning Nothing seems perfectly fine though.

@expede
Copy link
Member

expede commented Jun 15, 2018

@icidasset because nil can represent an actual value in Elixir, Algae.Maybe.new(nil) should return Algae.Maybe.Just{just: nil}. However, if you wanted to add a function like Algae.Maybe.from_nillable/1 or Algae.Maybe.new(value, nothing: nil) to distinguish nil -> Nothing for those that want it, that would work 👍 (Heck, it could even be a list of values)

EDIT: I actually kind of like the nothing: nil idea (above) since the user can define what they want to be taken as Nothing. Then a from_nillable could just be a synonym for new(value, nothing: nil).

@icidasset
Copy link
Member Author

Thanks @expede ! I've implemented your idea.

if value == nothing_value,
do: Nothing.new(),
else: Just.new(value)
end
Copy link
Member

@expede expede Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icidasset

Two very minor nitpicks:

Nothing missing in @spec

@spec new(any(), [nothing: any()]) :: Just.t() | Nothing.t()

(Also may as well add the expected key ☝️)

Pattern Matching

Suuuuper optional, but have you considered the following?

def new(nope, [nothing: nope]), do: Nothing.new()
def new(value, _), do: Just.new(value)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@expede Thanks! I didn't know you could match variables with pattern matching, neat! Spec is fixed as well.

@OvermindDL1
Copy link

OvermindDL1 commented Jul 2, 2018

Why not something more like this for new/2?

@spec new(err, [nothing: err]) :: Nothing.t(err)
@spec new(v, [nothing: err]) :: Just.t(v) when v != err

And adjust Just.t and Nothing.t as appropriate as well? (I think that is the right syntax for named typing, might need to make new types though?)

EDIT: Although the above would only work if err and value types are fully distinct, which might be enough for the top level new but would not be for the distinct cases, which is not an issue there anyway as they don't reference each other... Need tests...

@expede
Copy link
Member

expede commented Jul 2, 2018

@OvermindDL1 Yeah, the automagic type generation could use some work. At the time (way back in the .t days), I was unaware that you could embed types inside types.

Copy link
Member

@expede expede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome @icidasset — thanks!

@expede expede merged commit 52a662e into witchcrafters:master Jul 4, 2018
expede pushed a commit that referenced this pull request Jul 4, 2018
@icidasset icidasset deleted the maybe-nil branch July 5, 2018 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants